Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

split internal dates methods into separate functions #53692

Merged
merged 1 commit into from
Apr 20, 2024
Merged

split internal dates methods into separate functions #53692

merged 1 commit into from
Apr 20, 2024

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Mar 11, 2024 •
edited
Loading

These methods are clearly internal: not documented, and with unclear semantics in the general case.

They lead to weird results instead of errors:

julia> using Unitful

julia> yearmonthday(1u"°")
(0.0, 12.0, 31.017453292519917)

julia> using Measurements

julia> yearmonthday(100±1000)
(1.0 ± 0.0, 4.0 ± 0.0, 10.0 ± 1000.0)

So, would be best to really remove them from public functions (hope noone relies on them).

@aplavin
Copy link
Contributor Author

aplavin commented Apr 18, 2024

bump

@quinnj quinnj merged commit 8416647 into JuliaLang:master Apr 20, 2024
8 of 11 checks passed
@giordano
Copy link
Contributor

giordano commented Apr 20, 2024 •
edited
Loading

Loads of tests have been broken by this PR, it shouldn't have been merged, I'm going to revert it

giordano added a commit that referenced this pull request Apr 20, 2024
@giordano
Revert "split internal dates methods into separate functions (#53692)"
b0f4bad 
This reverts commit 8416647.
@giordano giordano mentioned this pull request Apr 20, 2024
Merged
giordano added a commit that referenced this pull request Apr 20, 2024
@giordano
Revert "split internal dates methods into separate functions" (#54159)
dd443f2 
Reverts #53692, this broke tests.
@giordano
Copy link
Contributor

@aplavin this PR is now reverted. You're welcome to resubmit again a working version, but all tests and checks were failing to your changes well before your bump, and this PR shouldn't have been merged in that state. Please be more careful next time and fix the breakage before bumping.

@aplavin
Copy link
Contributor Author

aplavin commented Apr 20, 2024

Sorry, I guess I just got used to tests often being broken unrelated to the changes in PRs, and didn't look into them...

These specific changes obviously don't introduce any new functionality. I submitted them for the sole reason of getting an error message instead of a weird result back, when passing unexpected types to these Dates functions.
Didn't expect this to produce test breakages at all, I guess moving these methods is not that important and isn't worth the risk. It's definitely not the worst case of "correctness bugs" :)

@LilithHafner LilithHafner added dates Dates, times, and the Dates stdlib module minor change Marginal behavior change acceptable for a minor release labels Apr 20, 2024
@LilithHafner
Copy link
Member

LilithHafner commented Apr 20, 2024 •
edited
Loading

I think these changes are appropriate, we just also need to adjust the CI tests to allow for them, and run nanosoldier to make sure people don't actually rely on them.

@aplavin aplavin mentioned this pull request Jul 30, 2024
Merged
@tecosaur tecosaur added the reverted This PR has since been reverted label Jul 31, 2024
@aplavin aplavin mentioned this pull request Aug 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quinnj quinnj quinnj approved these changes

Assignees
No one assigned
Labels
dates Dates, times, and the Dates stdlib module minor change Marginal behavior change acceptable for a minor release reverted This PR has since been reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aplavin @giordano @LilithHafner @quinnj @tecosaur